Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ensure support of the transport-nio by security plugin (HTTP) #16474

Merged
merged 2 commits into from
Nov 5, 2024
Merged

Ensure support of the transport-nio by security plugin (HTTP) #16474

merged 2 commits into from
Nov 5, 2024

Conversation

reta
Copy link
Collaborator

@reta reta commented Oct 24, 2024 •
edited
Loading

Description

Ensure support of the transport-nio by security plugin (HTTP only), changes in opensearch.yml:

http.type: nio-http-transport-secure

$ curl -kv https://localhost:9200/ -u admin:_ad0m#Ns_ --http1.1                                                                                                                                                                                                                              08:32:15 [39/437]
* Host localhost:9200 was resolved.                                                  
* IPv6: ::1                                                                          
* IPv4: 127.0.0.1                                                                    
*   Trying [::1]:9200...                                                             
* Connected to localhost (::1) port 9200                                             
* ALPN: curl offers http/1.1                                                         
* TLSv1.3 (OUT), TLS handshake, Client hello (1):
* TLSv1.3 (IN), TLS handshake, Server hello (2):
* TLSv1.3 (IN), TLS handshake, Encrypted Extensions (8):
* TLSv1.3 (IN), TLS handshake, Request CERT (13):
* TLSv1.3 (IN), TLS handshake, Certificate (11):
* TLSv1.3 (IN), TLS handshake, CERT verify (15):
* TLSv1.3 (IN), TLS handshake, Finished (20):
* TLSv1.3 (OUT), TLS change cipher, Change cipher spec (1):
* TLSv1.3 (OUT), TLS handshake, Certificate (11):
* TLSv1.3 (OUT), TLS handshake, Finished (20):
* SSL connection using TLSv1.3 / TLS_AES_128_GCM_SHA256 / x25519 / RSASSA-PSS
* ALPN: server accepted http/1.1                                                     
* using HTTP/1.x                                                                                                                                                                                                                                                                                                                                      
* Server auth using Basic with user 'admin'                                                                                                                                                                                                                                                                                                           
> GET / HTTP/1.1                                                                                                                                                                                                                                                                                                                                      
> Host: localhost:9200                                                               
> Authorization: Basic YWRtaW46X2FkMG0jTnNf
> User-Agent: curl/8.9.1                                                             
> Accept: */*                                                                        
>                                                                                    
* Request completely sent off                                                        
* TLSv1.3 (IN), TLS handshake, Newsession Ticket (4):
< HTTP/1.1 200 OK                                                                    
< X-OpenSearch-Version: OpenSearch/3.0.0 (opensearch)
< content-type: application/json; charset=UTF-8
< content-length: 576                                                                
<                                                                                    
{                                                                                    
  "name" : "host",                                              
  "cluster_name" : "opensearch",                                                     
  "cluster_uuid" : "ByTSGP1rSGOnak0LKonwWw",
  "version" : {                                                                      
    "distribution" : "opensearch",                                                   
    "number" : "3.0.0",                                                              
    "build_type" : "tar",                                                            
    "build_hash" : "9dd1a59847ad8c2716d002716521ac40afc69355",
    "build_date" : "2024-10-24T04:39:37.808432550Z",
    "build_snapshot" : false,                                                        
    "lucene_version" : "9.12.0",                                                     
    "minimum_wire_compatibility_version" : "2.18.0",
    "minimum_index_compatibility_version" : "2.0.0"
  },                                                                                 
  "tagline" : "The OpenSearch Project: https://opensearch.org/"
}

Related Issues

Closes #13245

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@github-actions github-actions bot added enhancement Enhancement or improvement to existing feature or request Plugins v2.19.0 Issues and PRs related to version 2.19.0 v3.0.0 Issues and PRs related to version 3.0.0 labels Oct 24, 2024
@github-actions GitHub Actions
Copy link
Contributor

❌ Gradle check result for 864d575: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@reta reta added backport 2.x Backport to 2.x branch and removed skip-changelog labels Oct 24, 2024
@github-actions GitHub Actions
Copy link
Contributor

❌ Gradle check result for 68d4f33: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions GitHub Actions
Copy link
Contributor

❌ Gradle check result for c56c85b: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions GitHub Actions
Copy link
Contributor

❌ Gradle check result for d295b86: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@reta reta force-pushed the issue-13245 branch 2 times, most recently from 0b169a7 to c99bf17 Compare October 29, 2024 16:00
@github-actions GitHub Actions
Copy link
Contributor

❌ Gradle check result for c99bf17: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions GitHub Actions
Copy link
Contributor

❌ Gradle check result for c99bf17: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions GitHub Actions
Copy link
Contributor

❌ Gradle check result for 6562fa5: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions GitHub Actions
Copy link
Contributor

❌ Gradle check result for 8aafff0: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions GitHub Actions
Copy link
Contributor

❌ Gradle check result for f4f30d4: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions GitHub Actions
Copy link
Contributor

❌ Gradle check result for 93a1491: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions GitHub Actions
Copy link
Contributor

❌ Gradle check result for 019c2ae: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

cwperks
cwperks reviewed Nov 4, 2024
View reviewed changes
Copy link
Member

@cwperks cwperks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @reta! I'll take a few passes at this PR and get feedback ASAP.

There was a pretty sizable change to the netty pipeline in 2.11 in these 2 PRs which should be extended to any http transport.

There is an automated test here which performs a stress test.

plugins/transport-nio/src/internalClusterTest/java/org/opensearch/http/nio/NioPipeliningIT.java Outdated Show resolved Hide resolved
@reta
Copy link
Collaborator Author

reta commented Nov 4, 2024

Nice work @reta! I'll take a few passes at this PR and get feedback ASAP.

Thanks @cwperks !

There was a pretty sizable change to the netty pipeline in 2.11 in these 2 PRs which should be extended to any http transport.

I think these improvements could go independently of each other, right?

@cwperks
Copy link
Member

cwperks commented Nov 4, 2024

I think we need to get that test passing w/ the security plugin to declare that this transport works w/ the security plugin. You can run it with the changes in this branch: cwperks/security#33

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 4, 2024

❌ Gradle check result for 7abf006: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@reta
Copy link
Collaborator Author

reta commented Nov 4, 2024

I think we need to get that test passing w/ the security plugin to declare that this transport works w/ the security plugin. You can run it with the changes in this branch: cwperks/security#33

This is not as straightforward sadly: the security plugin (at the moment) still only recognizes SecureNetty4HttpServerTransport (see please OpenSearchSecureSettingsFactory). We definitely could work towards removing these last pieces of knowledge from it.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 4, 2024

✅ Gradle check result for 7abf006: SUCCESS

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 4, 2024

❌ Gradle check result for dcb05d0: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 5, 2024

❌ Gradle check result for dcb05d0: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 5, 2024

❕ Gradle check result for dcb05d0: UNSTABLE

  • TEST FAILURES:
      1 org.opensearch.http.SearchRestCancellationIT.testAutomaticCancellationMultiSearchDuringQueryPhase
      1 org.opensearch.backwards.MixedClusterClientYamlTestSuiteIT.test {p0=cluster.health/10_basic/cluster health with closed index}

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

This was referenced Nov 5, 2024
Open
Open
Open
@cwperks
Copy link
Member

cwperks commented Nov 5, 2024

Thank you @reta! This PR looks good to me. I'm not a maintainer of this repo so I will leave a comment approval :shipit:

@reta reta merged commit b25e10a into opensearch-project:main Nov 5, 2024
38 checks passed
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch/backport-2.x
# Create a new branch
git switch --create backport/backport-16474-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 b25e10afb9e216c547a59409d909ec1ecae101ec
# Push it to GitHub
git push --set-upstream origin backport/backport-16474-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-16474-to-2.x.

@andrross
Copy link
Member

andrross commented Nov 5, 2024

Just curious @reta, is there any reason to prefer transport-nio over the Netty version? I'm wondering if this is something to consider removing in the next major version.

@reta
Copy link
Collaborator Author

reta commented Nov 5, 2024

Just curious @reta, is there any reason to prefer transport-nio over the Netty version? I'm wondering if this is something to consider removing in the next major version.

Thanks @andrross I think we've never benchmarked / compared these transports side by side. I will try to research the history of nio transport, thanks!

reta added a commit to reta/OpenSearch that referenced this pull request Nov 5, 2024
@reta
Ensure support of the transport-nio by security plugin (HTTP) (opense…
dad027f 
…arch-project#16474)

* Ensure support of the transport-nio by security plugin (HTTP)

Signed-off-by: Andriy Redko <[email protected]>

* Add header verifier and decompressor support of secure NIO transport variant

Signed-off-by: Andriy Redko <[email protected]>

---------

Signed-off-by: Andriy Redko <[email protected]>
(cherry picked from commit b25e10a)
@reta reta mentioned this pull request Nov 5, 2024
Merged
@andrross
Copy link
Member

andrross commented Nov 5, 2024

Thanks @reta. I'm always looking for opportunity to remove code!

reta added a commit that referenced this pull request Nov 5, 2024
@reta
Ensure support of the transport-nio by security plugin (HTTP) (#16474) (
047e78e 
#16565)

* Ensure support of the transport-nio by security plugin (HTTP)

Signed-off-by: Andriy Redko <[email protected]>

* Add header verifier and decompressor support of secure NIO transport variant

Signed-off-by: Andriy Redko <[email protected]>

---------

Signed-off-by: Andriy Redko <[email protected]>
(cherry picked from commit b25e10a)
@opensearch-ci-bot opensearch-ci-bot mentioned this pull request Nov 6, 2024
Open
@reta
Copy link
Collaborator Author

reta commented Nov 6, 2024

Thanks @reta. I'm always looking for opportunity to remove code!

@andrross traced it back to elastic/elasticsearch#27260 (which basically what I also suspected):

We are interested in introducing alternative transport options that do not depend on external libraries. From time to time we have had some friction with using netty related to buffer pooling, oom excetions, exception handling, unsafe, etc. We want to look explore if a homegrown transport could potentially meet our needs.

Not sure it is relevant these day, unless java.nio shines in some flows

@andrross andrross mentioned this pull request Dec 19, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cwperks cwperks cwperks left review comments

@andrross andrross andrross approved these changes

@peternied peternied Awaiting requested review from peternied peternied is a code owner

@anasalkouz anasalkouz Awaiting requested review from anasalkouz anasalkouz is a code owner

@ashking94 ashking94 Awaiting requested review from ashking94 ashking94 is a code owner

@Bukhtawar Bukhtawar Awaiting requested review from Bukhtawar Bukhtawar is a code owner

@CEHENKLE CEHENKLE Awaiting requested review from CEHENKLE CEHENKLE is a code owner

@dblock dblock Awaiting requested review from dblock dblock is a code owner

@dbwiddis dbwiddis Awaiting requested review from dbwiddis dbwiddis is a code owner

@gbbafna gbbafna Awaiting requested review from gbbafna gbbafna is a code owner

@jainankitk jainankitk Awaiting requested review from jainankitk jainankitk is a code owner

@kotwanikunal kotwanikunal Awaiting requested review from kotwanikunal kotwanikunal is a code owner

@linuxpi linuxpi Awaiting requested review from linuxpi linuxpi is a code owner

@mch2 mch2 Awaiting requested review from mch2 mch2 is a code owner

@msfroh msfroh Awaiting requested review from msfroh msfroh is a code owner

@nknize nknize Awaiting requested review from nknize nknize is a code owner

@owaiskazi19 owaiskazi19 Awaiting requested review from owaiskazi19 owaiskazi19 is a code owner

@Rishikesh1159 Rishikesh1159 Awaiting requested review from Rishikesh1159 Rishikesh1159 is a code owner

@sachinpkale sachinpkale Awaiting requested review from sachinpkale sachinpkale is a code owner

@saratvemulapalli saratvemulapalli Awaiting requested review from saratvemulapalli saratvemulapalli is a code owner

@shwetathareja shwetathareja Awaiting requested review from shwetathareja shwetathareja is a code owner

@sohami sohami Awaiting requested review from sohami sohami is a code owner

@VachaShah VachaShah Awaiting requested review from VachaShah VachaShah is a code owner

Assignees
No one assigned
Labels
backport 2.x Backport to 2.x branch backport-failed enhancement Enhancement or improvement to existing feature or request Plugins v2.19.0 Issues and PRs related to version 2.19.0 v3.0.0 Issues and PRs related to version 3.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[transport] Ensure support of the transport-nio by security plugin (HTTP)
3 participants
@reta @cwperks @andrross